feat(display) output status widget shows ble information when preferred#3236
feat(display) output status widget shows ble information when preferred#3236yrb wants to merge 2 commits intozmkfirmware:mainfrom
Conversation
| raise_zmk_preferred_transport_changed( | ||
| (struct zmk_preferred_transport_changed){.transport = preferred_transport}); |
There was a problem hiding this comment.
Why did the update_current_endpoint() call get removed? Shouldn't it be kept?
| raise_zmk_preferred_transport_changed( | |
| (struct zmk_preferred_transport_changed){.transport = preferred_transport}); | |
| update_current_endpoint(); | |
| raise_zmk_preferred_transport_changed( | |
| (struct zmk_preferred_transport_changed){.transport = preferred_transport}); |
There was a problem hiding this comment.
It seemed more consistent to let update_current_endpoint be called through the endpoint_listener (line 507), now there is an event. That subscription could be removed and update_current_endpoint could be called directly.
Though I would say that it should be called after raising the transport changed event so that if the endpoint does change, a subscriber sees the transport change event before the endpoint change event.
There was a problem hiding this comment.
got it, I couldn't see where this call had gone in the diff but that makes sense now.
Given that this new event affects details of the preferred transport feature (and is not just fired away for displays to listen to) you may want to split in that change into a separate commit to facilitate the review and hopefully speed up the merge
|
Can you provide pictures/screenshots of all the different cases now? (BLE preferred and connected/not connected, USB preferred and connected/not connected, preferred transport set to "none")
I may be misunderstanding this change since I'm not sure what changes you're making to all the cases listed above, but I explicitly chose to use the USB symbol followed by the close symbol when USB is preferred but the actual transport is none so that it would match the close symbol used for an inactive BLE connection. It looks like that has been removed in this change? I would be fine with changing the symbol used when the preferred transport is explicitly set to "none", though I'm not sure a minus symbol is any clearer. |
|
The logic was to make the display read as [actual connection] [preferred connection info], and to avoid the "X 1 X" display, the other way I also tested was changing the disconnected icon, but opted for the smallest change to 3.0 behavior. TRANSPORT_NONE sort of does this double duty between being a transport in its own right and disconnected from the preferred transport type when there is no fallback. It is this fallback behaviour that causes the wrinkle. For my personal use case my keyboard is connected via usb but also using BLE for secondary devices. So I end up in the state of actual connection being USB but the preferred begin BLE quite a bit, I find it makes this case more clear displaying "[usb] 1 X", to be uniform usb preferred and disconnected does end up as "- [usb]" which is not nice but uniform. I am not sure how common mixed usb/ble setups are in the community and having the output slightly more optimised in this direction. The USB disconnected case could be special cased out as well to make it the more appealing "[usb] X", which would be better for people that are not using BLE. After thinking about it further, I think my preferred change would be to remove the fallback to usb behaviour, so the endpoint is always your preferred or none. This removes the mixed usb/ble case. The display can then just be "[preferred transport] [connection state]". Making this PR almost unneeded, as I believe how it is currently would work well with the small fix of using active_profile_index. However, that seems like a much bigger change to current behaviour that I am not sure how people feel about. |
yrb
force-pushed
the
output_status
branch
2 times, most recently
from
bec3b24 to
4b969e7
Compare
|
I have changed the transport none symbol back to the close symbol since it now no longer tries to be explicit to the user about what the endpoint state is versus the preferred state. So the friendlier display of "[usb] X" is back in the case of disconnected usb. |
|
@joelspadin Here is a comparison of what the widget displays against what it changes to under this PR. For the example ble profile 1 is is connected, 3 is disconnected, and 2,4,5 are unbonded, the preferred state is ble. When the preferred state is usb the behaviour is unchanged. 
|
|
The USB connected, profile 1 cases for both old and new show the wireless icon. That doesn't seem right? Or when you say
|
|
All of those pictures in the boxes are with ble as the preferred transport, usb means usb is available as a fallback, no usb means it can't fall back to a usb connection. The profile one case is when ble is connected which works fine currently, hence it is the same in all cases. The usb disconnected at the bottom is usb preferred with no ble or usb connection available. Should have really left that out, but it was more for you to show that it is unchanged.
It is better than the current state where cycling through your connections and it just shows you a usb connection icon, and you have no idea what ble connection you have selected. Or if you don't have usb doesn't actually show you the active profile number. The display is basically telling you with the icon, what you are connected to. Then it is showing you the ble connection info when relevant. Relevant in this case being connected to ble or if you prefer to be connected to ble. It is not ideal, but the other options are either explicitly calling out the connection state, and preferred state which can look a little more confusing in the confined widget area. You would probably want to make these different widgets so they can be in different areas of the screen so there is better differentiation. The other option is disabling the fallback to usb/ble so you don't end up in these mixed modes. This is what I have done for my personal keyboards now. |
|
@yrb, it looks like the PR is going to take a while because the solution is apparently not obvious and the code is inconvenient to test :( Could you please open a separate PR with only you first commit adding the |
|
@snoyer The active display and getting info about the ble connection when usb is active seems to be something that does need a fix to improve the out of the box ux on the basic oled displays. I don't know how many people actually use this since niceviews seem quite popular. Another PR popped up that addressing the same underlying issues #3278 in different way and #2985 feels like it is trying to address the ux issue. @petejohanson do you have the bandwidth to make a comment about what you would like to see happen? |
It would indeed be nice if the default UX was improved but it is a non trivial question which is likely why there is multiple competing/related/overlaping PRs. My argument is that your first commit bdc1ab8 is a simple follow up to @joelspadin's #3140 that is needed for third-party display modules to properly react to |
yrb
force-pushed
the
output_status
branch
2 times, most recently
from
5134476 to
5e0faef
Compare
Added an event for the preferred transport changing, this is primarily to trigger display updates.
Shows the active ble connnection when it is the preferred connection this allows unbonded and inactive connections to be toggled through even if the keyboard has an active connection of another transport type.
3 participants
This improves the output widget when using it with usb and ble devices connected, by showing the ble connection information when it is active or prefered connection type.
This is primarily to fix the issue when you are connected over usb, the display will show the usb symbol rather than an unbound or disconnected ble connection. Giving limited feedback and making it hard to navigate connections using BLE_NXT/PRV.
When ble is the active or preferred transport ble connection information will be shown. In the case you have selected a ble connection but the fallback usb connection has been selected as the active connecteion, the usb symbol will be shown instead of the wifi symbol.
This is similar to an existing pull request #2985 which proposed to make the connection always visible to solve this usability issue. However, this does it by trying to make the information available when it is contextually relevant.
To get the display to update correctly when the preferred transport has changed, but it didn't cause a change in active endpoint the preferred_transport_changed event had to be added.
Comparison of current widget to the new widget
The current/new comparisons are all performed with BLE as the preferred transport.
USB - usb is available as a fallback connection
No USB - usb is unplugged, not available as a fallback
BLE profiles are in the following states
When the preferred state is usb the behaviour is unchanged by this PR.
PR check-list